feat(validator): FPI support in data store #1493
Conversation
Mirko-von-Leipzig
changed the title
bobbinth
left a comment
bobbinth
left a comment
There was a problem hiding this comment.
Looks good! Thank you! I left just one small nit inline.
Also, would be good to get a review from @igamigo. Maybe we can run client integration tests against this branch to confirm that transactions that do FPI calls can indeed be accepted by the node.
I tried running the tests from miden-client |
| if self.tx_inputs.account().id() == account_id { | ||
| // Get asset witnessess from native account. | ||
| Result::<Vec<_>, _>::from_iter(vault_keys.into_iter().map(|vault_key| { | ||
| match self.tx_inputs.account().vault().open(vault_key) { |
There was a problem hiding this comment.
The native account's witnesses are handled just like the ones for foreign accounts, i.e. they are added to the advice inputs on lazy loading, but not to the PartialVault. Any asset witnesses that are present in the partial vault at the beginning are also added to the advice inputs.
So, the advice inputs contain both the pre-provided asset witnesses and those fetched during lazy loading, and so we should read from the advice inputs and not from the partial vault.
The need for this should go away with 0xMiden/miden-base#2298 completely, at which point this could be replaced with unimplemented!.
Because of these subtle differences, I still think it would be good to add a test for this, e.g. one that has an FPI call, reads a storage map item lazily and has input notes with some assets, so the pre-fetch is executed.
And then all of these annoying details should be abstracted away once we have a re-execution API exposed in base directly: 0xMiden/miden-base#2293.
There was a problem hiding this comment.
0xMiden/miden-base#2298 has been merged so I have moved this to unimplemented
There was a problem hiding this comment.
0xMiden/miden-base#2298 is not a part of v0.13.0 release - it went into next. So, we can't make use of it in this PR.
PhilippGackstatter
left a comment
PhilippGackstatter
left a comment
There was a problem hiding this comment.
Looks good to me!
bobbinth
left a comment
bobbinth
left a comment
There was a problem hiding this comment.
Looks good! Thank you! I'll merge as as - but let's consider adding a test in a follow-up PR.
Context
The Validator's
TransactionInputsDataStoredoes not handle foreign accounts for the purposes of the following functions ofDataStoretrait:get_foreign_account_inputs()NOTE: This will remain in draft until miden-base has the
TransactionIniputs::read_foreign_account_inputs()functionality merged into next.A followup PR will be made to add similar functionality to the
NtxDataStore.Relates to #1387 and #1305.
Changes
TransactionInputsDataStoreto handle foreign accounts throughself.tx_inputs.